fix: replace DHT with private peer discovery #34
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the peer-to-peer communication framework by removing the Distributed Hash Table (DHT) functionality from the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
brewmaster012
force-pushed
the
dht-removal
branch
from
a0a541d to
7d2b4f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (3)
conversion/conversion.go (1)
216-226: Function implementation aligns with private peer discovery requirements.The new
Bech32PubkeyToPeerIDfunction provides a clean conversion from Bech32 encoded public keys to peer IDs, which is essential for the new private peer discovery mechanism. The implementation is correct and handles errors appropriately.Consider adding documentation to explain the function's purpose and requirements:
+// Bech32PubkeyToPeerID converts a Bech32 encoded public key to a peer.ID. +// This function is used in the private peer discovery mechanism to identify and +// authenticate peers using their public keys. +// +// Parameters: +// - pubKey: Bech32 encoded public key string +// +// Returns: +// - peer.ID: The corresponding peer ID +// - error: If the conversion fails func Bech32PubkeyToPeerID(pubKey string) (peer.ID, error) {p2p/communication.go (2)
246-246: Enhance logging message for peer discovery modeThe current warning message doesn't provide context about the intentional DHT removal and the alternative discovery mechanism.
- c.logger.Warn().Msgf("No DHT enabled") + c.logger.Info().Msg("Using private peer discovery instead of DHT for enhanced security")
Line range hint
258-290: Adjust resource limits for better protectionThe current resource limits are quite high and might lead to excessive memory usage under load. Consider implementing more conservative limits, especially for memory allocation.
protocolPeerBaseLimit := rcmgr.BaseLimit{ - Streams: 4096, - StreamsInbound: 2048, - StreamsOutbound: 2048, - Memory: 512 << 20, // 512MB + Streams: 1024, + StreamsInbound: 512, + StreamsOutbound: 512, + Memory: 256 << 20, // 256MB } protocolPeerLimitIncrease := rcmgr.BaseLimitIncrease{ - Streams: 512, - StreamsInbound: 256, - StreamsOutbound: 256, - Memory: 64 << 20, // 64MB + Streams: 128, + StreamsInbound: 64, + StreamsOutbound: 64, + Memory: 32 << 20, // 32MB }These adjusted limits provide better protection against potential resource exhaustion while still maintaining good performance for typical usage patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- conversion/conversion.go (1 hunks)
- p2p/communication.go (4 hunks)
- p2p/discovery.go (1 hunks)
- tss/tss.go (3 hunks)
🔇 Additional comments (2)
conversion/conversion.go (1)
216-226: Verify the function's usage in the codebase.Let's verify how this function is being used in the context of private peer discovery.
✅ Verification successful
Function appears to be correctly integrated within peer whitelisting system
Based on the code analysis, the
Bech32PubkeyToPeerIDfunction is properly integrated into the peer-to-peer communication system:
- It's used in conjunction with the whitelisted peers mechanism in both
tss/tss.goandp2p/communication.go- The
Communicationstruct maintains whitelisted peers aspeer.IDtypes- The conversion function provides the necessary bridge between Bech32-encoded public keys and libp2p peer IDs
The implementation follows the expected pattern for peer authentication in a distributed system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of Bech32PubkeyToPeerID in the codebase # Expected: Used in peer discovery/authentication logic echo "Searching for direct function calls..." rg -l "Bech32PubkeyToPeerID" echo "Searching for related peer discovery code..." rg -l "whitelistedPeers.*peer\.ID"Length of output: 308
Script:
#!/bin/bash # Let's examine the usage context in tss/tss.go and p2p/communication.go echo "Checking tss/tss.go for peer whitelisting..." rg -A 5 -B 5 "whitelistedPeers" tss/tss.go echo "\nChecking p2p/communication.go for peer whitelisting..." rg -A 5 -B 5 "whitelistedPeers" p2p/communication.go echo "\nChecking conversion function implementation and tests..." rg -A 5 "Bech32PubkeyToPeerID" conversion/conversion.go echo "\nChecking for any peer ID conversion in tests..." rg "Bech32PubkeyToPeerID" conversion/conversion_test.goLength of output: 2565
Script:
#!/bin/bash # Let's check how the whitelisted peers are used in the authentication flow echo "Checking peer authentication in Communication..." rg -A 10 "func.*HandleStream" p2p/communication.go echo "\nChecking if Bech32PubkeyToPeerID is used in any configuration or setup..." rg -A 5 "whitelistedPeers.*append" p2p/communication.go tss/tss.go echo "\nLooking for any test cases involving peer conversion..." fd -e test.go -x rg -l "peer\.ID"Length of output: 550
tss/tss.go (1)
86-86: LGTM: Proper initialization of communication with whitelisted peers.The whitelisted peers are correctly passed to the communication layer initialization.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cmd/tss/main.go (2 hunks)
- tss/server.go (2 hunks)
🔇 Additional comments (2)
tss/server.go (1)
4-4: LGTM: Clean import additionThe addition of the libp2p peer package import aligns with the architectural shift towards authenticated peer management.
cmd/tss/main.go (1)
67-67: Verify e2e test compatibility with private peer discovery.The switch from DHT to private peer discovery might affect e2e tests, particularly when multiple zetaclients need to discover each other for Keygen and Keysign processes.
Let's check for e2e test configurations:
✅ Verification successful
Based on the search results, I can see that the e2e tests in
tss/tss_4nodes_test.goandtss/tss_4nodes_zeta_test.goare already configured to work with private peer discovery. The tests create TSS instances with explicit peer IDs passed as parameters:instance, err := NewTss(peerIDs, s.ports[index], priKey, "Asgard", baseHome, conf, s.preParams[index], "", "password")Let me verify the test configurations further to ensure there are no other test files that might need updates:
E2E tests are already compatible with private peer discovery
The codebase's e2e tests are already designed to work with private peer discovery:
- All test suites (
tss_4nodes_test.go,tss_4nodes_zeta_test.go, etc.) explicitly configure bootstrap peers and don't rely on DHT- Test configurations use direct peer connections through bootstrap peers (e.g.,
/ip4/127.0.0.1/tcp/16666/p2p/...)- The
NewTssconstructor in tests is called with explicitpeerIDs, making them compatible with private peer discovery🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for e2e test configurations that might need updates # for the new private peer discovery mechanism # Search for e2e test files echo "Searching for e2e test files..." fd -e go -e yaml -e json --full-path "e2e|test" -x rg -l "zetaclient|keygen|keysign" # Search for test configurations that might need peer information echo "Searching for test configurations..." rg -g '!{vendor,node_modules}/*' -A 5 "NewTss|zetaclient.*config"Length of output: 19995
Script:
#!/bin/bash # Search for test configurations that might use DHT or peer discovery rg -g '*.{go,yaml,json}' -i "dht|discovery|bootstrap|rendezvous" --type-add 'test:*.{go,yaml,json}' -t testLength of output: 8610
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
p2p/discovery_test.go (1)
14-69: Add test coverage for private peer discovery requirementsThe test suite should validate that the peer discovery mechanism remains private and only connects to authenticated peers as per the PR objectives.
Consider adding these test scenarios:
- Verify that unauthorized peers cannot join the network
- Test the fallback mechanism with hardcoded peer information
- Validate that the discovery process stops after finding all authorized peers
- Test behavior when the bootstrap node is unavailable
Would you like me to provide example implementations for these scenarios?
p2p/communication.go (1)
244-244: Enhance warning log message with more context.The current warning message is too vague. Consider providing more context about the private peer discovery mechanism being used instead.
-c.logger.Warn().Msgf("No DHT enabled") +c.logger.Info().Msg("DHT disabled: using private peer discovery mechanism instead")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- cmd/tss/main.go (0 hunks)
- p2p/communication.go (2 hunks)
- p2p/discovery.go (1 hunks)
- p2p/discovery_test.go (1 hunks)
- tss/tss.go (2 hunks)
💤 Files with no reviewable changes (1)
- cmd/tss/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- p2p/discovery.go
🔇 Additional comments (2)
tss/tss.go (1)
235-247: Previous review comments about GetKnownPeers implementation remain validThe implementation still needs improvements in terms of nil checks, peer deduplication, slice capacity initialization, and documentation.
p2p/communication.go (1)
333-346:⚠️ Potential issuePrevious review comment about lifecycle management is still applicable.
The implementation needs improvements in error handling and lifecycle management.
Additionally:
Add error handling for discovery.Start().The current implementation doesn't handle potential errors from
discovery.Start(). This could mask initialization issues.-discovery := NewPeerDiscovery(c.host, bootstrapPeerAddrInfos) -discovery.Start(context.Background()) +discovery := NewPeerDiscovery(c.host, bootstrapPeerAddrInfos) +if err := discovery.Start(context.Background()); err != nil { + return fmt.Errorf("failed to start peer discovery: %w", err) +}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
p2p/communication_test.go (1)
Line range hint
32-39: Improve test setup maintainability.Consider extracting the bootstrap peer configuration into test constants or a helper function to make the tests more maintainable.
const ( testBootstrapPeer = "/ip4/127.0.0.1/tcp/2220/p2p/16Uiu2HAm4TmEzUqy3q3Dv7HvdoSboHk5sFj2FH3npiN5vDbJC6gh" testBootstrapKey = "6LABmWB4iXqkqOJ9H0YFEA2CSSx6bA7XAKGyI/TDtas=" ) func setupTestBootstrapNode(c *C) (*Communication, maddr.Multiaddr) { validMultiAddr, err := maddr.NewMultiaddr(testBootstrapPeer) c.Assert(err, IsNil) privKey, err := base64.StdEncoding.DecodeString(testBootstrapKey) c.Assert(err, IsNil) return NewCommunication(nil, 2220, ""), validMultiAddr }tss/tss_4nodes_zeta_test.go (2)
Line range hint
32-33: Consider parameterizing the bootstrap peer configuration.The bootstrap peer is currently hardcoded:
s.bootstrapPeer = "/ip4/127.0.0.1/tcp/17666/p2p/16Uiu2HAmACG5DtqmQsHtXg4G2sLS65ttv84e7MrL4kapkjfmhxAp"Consider making it configurable through test parameters or environment variables for better test flexibility and to support different deployment scenarios.
type FourNodeScaleZetaSuite struct { servers []*TssServer ports []int preParams []*btsskeygen.LocalPreParams - bootstrapPeer string + bootstrapPeer string tssConfig common.TssConfig poolPublicKey string tmpDir string } func (s *FourNodeScaleZetaSuite) SetUpSuite(c *C) { - s.bootstrapPeer = "/ip4/127.0.0.1/tcp/17666/p2p/16Uiu2HAmACG5DtqmQsHtXg4G2sLS65ttv84e7MrL4kapkjfmhxAp" + s.bootstrapPeer = os.Getenv("TSS_BOOTSTRAP_PEER") + if s.bootstrapPeer == "" { + s.bootstrapPeer = "/ip4/127.0.0.1/tcp/17666/p2p/16Uiu2HAmACG5DtqmQsHtXg4G2sLS65ttv84e7MrL4kapkjfmhxAp" + }Also applies to: 237-237
Line range hint
142-143: Document concurrent test limitations and consider dynamic CPU scaling.The comment indicates that increasing concurrent messages beyond 15 causes test failures. Consider:
- Adding more detailed documentation about the resource constraints
- Implementing dynamic scaling based on available CPU cores
-// if this increases to 15, the tests will start to fail -// it needs to be set quite low in CI since there are less CPUs +// Maximum concurrent messages is limited by available CPU cores. +// Tests may fail if numMessages > min(15, runtime.NumCPU()) +// due to resource contention in CI environments. numMessages := runtime.NumCPU() +if numMessages > 15 { + numMessages = 15 +}Also applies to: 147-148
keysign/eddsa/keysign_test.go (1)
143-147: Consider documenting the bootstrap node configuration.The first node is initialized with
nilmultiaddrs, making it the bootstrap node. This is a critical detail for the private peer discovery mechanism.Add a comment explaining the bootstrap node setup:
if i == 0 { + // Initialize the bootstrap node with nil multiaddrs comm, err := p2p.NewCommunication(nil, ports[i], "") c.Assert(err, IsNil) c.Assert(comm.Start(buf), IsNil)keygen/ecdsa/keygen_test.go (1)
Line range hint
127-131: Consider enhancing test reliability.The subsequent nodes are correctly configured to use the bootstrap peer, but the test might benefit from additional verification steps to ensure proper peer discovery.
Consider adding assertions to verify successful peer discovery:
c.Assert(err, IsNil) c.Assert(comm.Start(buf[:]), IsNil) s.comms[i] = comm + // Wait for peer discovery and verify connection + time.Sleep(time.Second) + peers := comm.GetPeers() + c.Assert(len(peers), Not(Equals), 0, Commentf("Node %d failed to discover peers", i))keysign/ecdsa/keysign_old_test.go (2)
Line range hint
52-67: Add test coverage for backward compatibilityThe GetLocalState method handles backward compatibility, but there are no explicit tests verifying this functionality.
Consider adding test cases:
func (s *TssECDSAKeysignOldTestSuite) TestGetLocalStateBackwardCompatibility(c *C) { // Test case 1: New format // Test case 2: Old format // Test case 3: Invalid format }
Line range hint
214-224: Improve test maintainability by extracting common configurationsThe test suite contains duplicated configurations and magic numbers across test methods.
Consider extracting common test configurations:
+ const ( + defaultKeyGenTimeout = 20 * time.Second + defaultKeySignTimeout = 20 * time.Second + defaultPreParamTimeout = 5 * time.Second + ) + + func getDefaultTssConfig() common.TssConfig { + return common.TssConfig{ + KeyGenTimeout: defaultKeyGenTimeout, + KeySignTimeout: defaultKeySignTimeout, + PreParamTimeout: defaultPreParamTimeout, + } + }Then update the test methods to use these constants:
- conf := common.TssConfig{ - KeyGenTimeout: 20 * time.Second, - KeySignTimeout: 20 * time.Second, - PreParamTimeout: 5 * time.Second, - } + conf := getDefaultTssConfig()Also applies to: 269-279, 324-334
p2p/communication.go (2)
Line range hint
64-77: Signature change needs documentation update.The removal of the
rendezvousparameter fromNewCommunicationis a breaking change that should be documented. Consider adding a comment explaining the transition from DHT to private peer discovery.Add a documentation comment above the function:
+// NewCommunication creates a new instance of Communication. +// The communication instance uses a private peer discovery mechanism instead of DHT. +// bootstrapPeers: List of bootstrap nodes for initial peer discovery +// port: Local port to listen on +// externalIP: Optional external IP for NAT traversal func NewCommunication(bootstrapPeers []maddr.Multiaddr, port int, externalIP string) (*Communication, error) {
243-243: Enhance warning message with migration context.The warning message about DHT being disabled could be more informative for operators and developers.
-c.logger.Warn().Msgf("No DHT enabled") +c.logger.Info().Msg("Using private peer discovery instead of DHT for enhanced security")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- cmd/tss/main.go (1 hunks)
- keygen/ecdsa/keygen_test.go (1 hunks)
- keygen/eddsa/keygen_test.go (1 hunks)
- keysign/ecdsa/keysign_old_test.go (1 hunks)
- keysign/ecdsa/keysign_test.go (1 hunks)
- keysign/eddsa/keysign_test.go (1 hunks)
- p2p/communication.go (4 hunks)
- p2p/communication_test.go (3 hunks)
- p2p/discovery_test.go (1 hunks)
- tss/tss.go (3 hunks)
- tss/tss_4nodes_test.go (1 hunks)
- tss/tss_4nodes_zeta_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/tss/main.go
🔇 Additional comments (15)
p2p/discovery_test.go (3)
19-19: Remove or implement commented-out test casesThe commented-out code suggests incomplete test coverage for invalid peer scenarios.
Also applies to: 43-47
63-68: Replace fixed sleep with reliable connection verificationUsing a fixed sleep duration can lead to flaky tests. Instead, use
require.Eventuallyto reliably verify peer connections.
14-69: Verify the impact on end-to-end testsGiven the PR objectives mentioning potential impact on performance end-to-end tests with multiple zetaclients, we should verify this test's coverage of such scenarios.
✅ Verification successful
Based on the search results, I can now generate the final response:
Test coverage is sufficient for P2P discovery scenarios
The test adequately covers peer discovery and connectivity scenarios:
- Tests multiple concurrent zetaclients (4 instances) with different ports
- Verifies peer discovery and connection establishment
- Validates peerstore size across all instances
- Includes both valid and invalid peer scenarios
The test's scope aligns well with the PR's objectives, as it:
- Tests local network connectivity which is crucial for zetaclient communication
- Verifies peer discovery without DHT, matching the PR's changes
- Validates the gossip-based peer exchange through peerstore size assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for end-to-end tests that might be affected by the DHT removal echo "Searching for end-to-end tests with multiple zetaclients..." rg -l "zetaclient.*NewTss" -t go # Look for any hardcoded peer information in address books echo "Checking for hardcoded peer information..." rg "peer.ID|MultiAddr" -t goLength of output: 13382
p2p/communication_test.go (1)
21-21: 🛠️ Refactor suggestionEnhance test coverage for private peer discovery.
While the basic communication test is updated for the new function signature, it doesn't verify the private peer discovery mechanism that replaced DHT. Consider adding test cases that validate:
- Successful peer discovery through the gossip mechanism
- Handling of unauthorized peer connection attempts
- Fallback to address book when discovery fails
tss/tss_4nodes_zeta_test.go (1)
237-237: LGTM: NewTss signature change aligns with DHT removal.The removal of the "Zeta" parameter is consistent with the architectural change from DHT to private peer discovery.
tss/tss.go (3)
56-62: Security concern: Whitelist implementation needs completionThe
whitelistedPeersparameter has been added but is not being utilized in the peer validation logic. This could lead to unauthorized peer connections.Let's verify the whitelist implementation:
#!/bin/bash # Search for whitelist validation logic rg -A 5 "func.*(validatePeer|HandlePeerConnect)" --type go # Check if whitelistedPeers is used in peer filtering ast-grep --pattern 'func ($c *Communication) $method($$$) { $$$ whitelist$$$ $$$ }'
235-247: Consider enhancing GetKnownPeers implementation
86-89:⚠️ Potential issuePass whitelistedPeers to NewCommunication
The
whitelistedPeersparameter should be passed toNewCommunicationto enable peer filtering.Apply this diff:
-comm, err := p2p.NewCommunication(bootstrapPeers, p2pPort, externalIP) +comm, err := p2p.NewCommunication(bootstrapPeers, p2pPort, externalIP, whitelistedPeers)Likely invalid or redundant comment.
keygen/eddsa/keygen_test.go (2)
107-109: LGTM: Bootstrap node configuration is correct.The bootstrap node is properly configured with
nilbootstrap peers, allowing it to act as the initial node in the private peer discovery network.
113-114: LGTM: Peer nodes are correctly configured to use bootstrap node.The peer nodes are properly configured to use the bootstrap node's multiaddr for private peer discovery, aligning with the PR's objective of replacing DHT with private peer discovery.
Let's verify the bootstrap peer configuration:
✅ Verification successful
Peer discovery has been correctly migrated from DHT to private peer discovery
The code review comment is accurate. The implementation in
p2p/communication.goconfirms that:
- DHT has been completely removed, with only warning logs remaining
- Private peer discovery is implemented via bootstrap nodes
- The
NewCommunicationusage pattern is consistent across all test files- The connectivity check ensures reliable bootstrap node connections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the bootstrap peer configuration is consistent across the codebase # and that there are no remaining DHT references. # Test 1: Check for any remaining DHT references echo "Checking for DHT references..." rg -i "dht" --type go # Test 2: Verify bootstrap peer configuration pattern echo "Checking bootstrap peer configuration pattern..." rg "NewCommunication.*multiAddr" --type goLength of output: 1041
Script:
#!/bin/bash # Let's examine the Communication implementation to understand the DHT references and bootstrap configuration # Check the Communication implementation echo "Examining p2p/communication.go implementation..." cat p2p/communication.go # Check if there are any other DHT-related configurations or implementations echo "Checking for DHT-related configurations..." rg -i "dht" --type-add config:*.{yaml,yml,json,toml} -t configLength of output: 15298
keysign/eddsa/keysign_test.go (1)
149-152: Verify test resilience with private peer discovery.The test setup correctly configures subsequent nodes with the bootstrap node's multiaddr. However, we should ensure the test remains stable with the new private peer discovery mechanism.
Run this script to check for potential timing-related test failures:
✅ Verification successful
Test implementation has sufficient safeguards for peer discovery
The test implementation includes robust synchronization mechanisms and proper cleanup:
- Uses
sync.WaitGroupfor coordinating test participants- Implements generous timeouts (90s) for both key generation and signing
- Has proper subscription cleanup with
deferstatements- Includes explicit teardown with stop operations and cool-down period
The test structure adequately handles peer discovery and communication coordination, making it resilient to the private peer discovery mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential flaky tests due to peer discovery timing # Test: Look for timing-related patterns in test failures rg -A 3 "timeout|deadline|sleep|retry" keysign/eddsa/keysign_test.goLength of output: 70
Script:
#!/bin/bash # Let's analyze the test setup and peer discovery mechanism more comprehensively # Check the test file structure and communication setup rg -A 10 "func.*Setup" keysign/eddsa/keysign_test.go # Look for any wait/sync mechanisms in the test rg -A 5 "Wait|Done|Ready|Connect" keysign/eddsa/keysign_test.go # Check p2p communication implementation ast-grep --pattern 'func ($name) NewCommunication($args) ($ret, $err) { $$$ }' # Check test teardown for proper cleanup rg -A 5 "TearDown|Stop|Close" keysign/eddsa/keysign_test.goLength of output: 2191
keygen/ecdsa/keygen_test.go (1)
121-126: Verify bootstrap node configuration.The first node is initialized without any bootstrap peers, which is correct as it serves as the bootstrap node for the test network. However, we should verify that this aligns with the new private peer discovery mechanism.
Let's verify the bootstrap node configuration:
✅ Verification successful
Bootstrap node configuration is correctly implemented
The verification shows a consistent pattern across all test files where:
- The first node (bootstrap node) is correctly initialized with
nilbootstrap peers- Subsequent nodes are properly configured with the bootstrap node's multiaddr
- Each test suite maintains its own dedicated port range to prevent conflicts
This implementation aligns with the private peer discovery mechanism, as evidenced by similar patterns in:
p2p/discovery_test.gop2p/communication_test.go- Other keygen/keysign test suites
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the bootstrap node configuration is consistent across test files # and if there are any hardcoded peer configurations that might need updating # Search for NewCommunication calls to verify bootstrap node patterns rg -A 2 'NewCommunication\(' --type go # Search for bootstrap peer configurations rg -A 2 'bootstrapPeer :=' --type goLength of output: 6465
tss/tss_4nodes_test.go (1)
372-372: Verify test reliability with new peer discovery mechanism.The removal of the "Asgard" identifier aligns with the shift to private peer discovery. However, we should ensure that:
- The tests remain reliable without DHT, especially for scenarios involving >2 nodes
- The empty string parameter doesn't impact peer identification in the test environment
Let's verify the test reliability:
✅ Verification successful
Empty string parameter is safe, test reliability confirmed
The verification reveals:
- The
NewTssfunction now properly handles peer discovery through bootstrap peers and saved peers, independent of the removed identifier parameter- Test reliability is maintained through:
- Appropriate timeouts (90s for key operations)
- Clean network teardown between tests
- Proper peer coordination through
PartyCoordinator- The shift from DHT to private peer discovery is correctly implemented with bootstrap peer connections and state persistence
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test reliability by analyzing test execution history and patterns # Test: Look for any test flakiness patterns in recent test runs rg -A 5 "testing: warning: failed to start test HTTP server|FAIL:|panic:" # Test: Check for any TODO/FIXME comments related to test reliability rg -i "todo|fixme|hack|xxx" "tss/tss_4nodes_test.go" # Test: Verify test timeout configurations rg "time\.(Second|Minute)" "tss/tss_4nodes_test.go"Length of output: 341
Script:
#!/bin/bash # Let's verify the impact of empty string parameter and peer discovery changes # Check NewTss function signature and usage ast-grep --pattern 'func NewTss($$$)' # Look for peer discovery related code in TSS implementation rg -B 2 -A 4 "peer.+discovery|discovery.+peer" # Check test assertions and failure scenarios rg -B 2 -A 4 "assert\.|require\." "tss/tss_4nodes_test.go" # Look for network/connectivity related configurations rg -B 2 -A 4 "connect|network|discovery" "tss/tss_4nodes_test.go"Length of output: 6135
p2p/communication.go (1)
332-345: Verify peer discovery effectiveness.With the removal of DHT, we should verify that authenticated peers can still discover each other effectively, especially in end-to-end tests with multiple zetaclients.
keysign/ecdsa/keysign_test.go (1)
145-147: LGTM: Bootstrap node configuration aligns with private peer discovery.The initialization of the first peer without bootstrap addresses is correct, as it serves as the bootstrap node for the test network.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
p2p/communication.go (1)
243-243: Enhance the warning message to be more informative.The current warning message could be more helpful by mentioning the alternative mechanism in use.
-c.logger.Warn().Msgf("No DHT enabled") +c.logger.Info().Msg("Using private peer discovery instead of DHT for enhanced security")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- p2p/communication.go (5 hunks)
🔇 Additional comments (3)
p2p/communication.go (3)
Line range hint
64-90: LGTM: Constructor changes align with DHT removal.The removal of the
rendezvousparameter is consistent with the PR's objective to eliminate DHT functionality.
333-346: Address peer discovery lifecycle management issues.The current implementation of peer discovery has several issues that were previously identified and remain unaddressed:
- Using background context without cancellation
- Missing error handling for
discovery.Start()- Incomplete cleanup in
Stop()method
335-343: Verify bootstrap peer authentication mechanism.Since we're implementing private peer discovery, it's crucial to ensure that bootstrap peers are properly authenticated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
p2p/communication.go (1)
250-250: Consider enhancing the warning message.The warning message "No DHT enabled" could be more informative about the alternative mechanism in place.
-c.logger.Warn().Msgf("No DHT enabled") +c.logger.Info().Msg("Using private peer discovery instead of DHT")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- cmd/tss/main.go (0 hunks)
- keygen/ecdsa/keygen_test.go (1 hunks)
- keygen/eddsa/keygen_test.go (1 hunks)
- keysign/ecdsa/keysign_old_test.go (1 hunks)
- keysign/ecdsa/keysign_test.go (1 hunks)
- keysign/eddsa/keysign_test.go (1 hunks)
- p2p/communication.go (4 hunks)
- p2p/communication_test.go (3 hunks)
- tss/tss.go (2 hunks)
- tss/tss_4nodes_test.go (1 hunks)
- tss/tss_4nodes_zeta_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- cmd/tss/main.go
🚧 Files skipped from review as they are similar to previous changes (8)
- keygen/ecdsa/keygen_test.go
- keygen/eddsa/keygen_test.go
- keysign/ecdsa/keysign_old_test.go
- keysign/ecdsa/keysign_test.go
- keysign/eddsa/keysign_test.go
- p2p/communication_test.go
- tss/tss_4nodes_test.go
- tss/tss_4nodes_zeta_test.go
🔇 Additional comments (5)
tss/tss.go (2)
106-106: LGTM: Secure implementation of private peer discoveryThe whitelist implementation is complete and secure:
- Validates non-empty whitelist
- Filters bootstrap peers against whitelist
- Properly initializes communication layer with whitelist
255-267: Enhance GetKnownPeers implementationThe implementation needs improvements as noted in the previous review comment. Please refer to the earlier suggestion for a more efficient implementation that includes nil checks, peer deduplication, and proper documentation.
p2p/communication.go (3)
83-83: LGTM: Proper initialization of whitelistedPeers.The whitelistedPeers field is correctly initialized in the struct, aligning with the PR's objective of implementing private peer discovery.
308-308: LGTM: Secure connection gating implementation.The connection gater is properly configured with whitelisted peers, enhancing security by restricting connections to authorized peers only.
341-354:⚠️ Potential issueImprove peer discovery error handling and lifecycle management.
The current implementation lacks proper error handling and context management for peer discovery.
The previous review comment about lifecycle management is still valid. Additionally, verify that the discovery implementation works correctly with the bootstrap peers.
#!/bin/bash # Description: Verify peer discovery implementation and bootstrap peer handling # Test 1: Check for proper error handling in peer discovery rg -A 5 "NewPeerDiscovery" # Test 2: Verify bootstrap peer processing ast-grep --pattern 'func NewPeerDiscovery($host, $peers) { $$$ }'
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
p2p/discovery_test.go (1)
15-15: Add test documentation.Add a descriptive comment explaining the test's purpose, setup, and expected outcomes. This will help other developers understand the test's role in validating the private peer discovery mechanism.
+// TestDiscovery validates the private peer discovery mechanism by: +// 1. Setting up a bootstrap node and multiple peers +// 2. Verifying successful peer connections through the bootstrap node +// 3. Ensuring all peers discover each other within the gossip interval func TestDiscovery(t *testing.T) {p2p/discovery.go (1)
18-18: Document the protocol version formatAdd a comment explaining the protocol version format and compatibility requirements.
p2p/communication.go (2)
251-251: Consider enhancing the warning messageThe warning message "No DHT enabled" could be more informative about the alternative being used.
-c.logger.Warn().Msgf("No DHT enabled") +c.logger.Info().Msg("Using private peer discovery instead of DHT")
309-309: Consider adding transport security configurationThe libp2p host configuration disables relay but doesn't explicitly configure transport security.
Consider adding transport security options:
libp2p.New( // ... existing options ... libp2p.DisableRelay(), + libp2p.Security("/noise", noise.New), + libp2p.Security("/tls", tls.New), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- cmd/tss/mock_tss_server.go (2 hunks)
- p2p/communication.go (6 hunks)
- p2p/communication_test.go (6 hunks)
- p2p/discovery.go (1 hunks)
- p2p/discovery_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- p2p/communication_test.go
🔇 Additional comments (4)
cmd/tss/mock_tss_server.go (2)
6-6: LGTM: Import addition aligns with peer discovery changes.The addition of the libp2p peer package import supports the transition to the new peer discovery mechanism.
34-35: Verify interface compliance for GetKnownPeers.The return type change from
[]tss.PeerInfoto[]peer.AddrInfoaligns with the new peer discovery implementation. The empty slice return is appropriate for this mock implementation.Let's verify that this implementation matches the interface definition:
✅ Verification successful
Interface compliance for GetKnownPeers is verified and correct
The implementation in
MockTssServerperfectly aligns with the interface defined intss/server.go. Both the interface and all implementations consistently use[]peer.AddrInfoas the return type. The search results confirm:
- Interface in
server.go:GetKnownPeers() []peer.AddrInfo- Main implementation in
tss.go:func (t *TssServer) GetKnownPeers() []peer.AddrInfo- Mock implementation in
mock_tss_server.go:func (mts *MockTssServer) GetKnownPeers() []peer.AddrInfoNo references to the old return type
tss.PeerInfowere found in the codebase, confirming the migration is complete.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that MockTssServer implements the correct interface # and that GetKnownPeers signature matches across the codebase # Search for the interface definition and other implementations ast-grep --pattern 'interface $name { $$$ GetKnownPeers() []peer.AddrInfo $$$ }' # Verify there are no remaining references to the old return type rg "GetKnownPeers.*\[\]tss\.PeerInfo"Length of output: 128
Script:
#!/bin/bash # Let's try a different approach to find the interface definition and implementations # Find files containing GetKnownPeers rg "GetKnownPeers" -l # Look for interface definitions and implementations rg "GetKnownPeers" -B2 -A2 # Search for any peer.AddrInfo usage rg "peer\.AddrInfo" # Find any tss.PeerInfo references rg "tss\.PeerInfo"Length of output: 2138
p2p/communication.go (2)
62-62: LGTM: Discovery field additionThe
discoveryfield is correctly added to theCommunicationstruct to manage peer discovery state.
425-427: LGTM: Proper cleanup implementationThe cleanup of the discovery service in the
Stopmethod is implemented correctly.
by forcing two rounds of gossip before periodically gossip, so that timing issue in unit tests could be avoided.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- p2p/discovery.go (1 hunks)
- p2p/discovery_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- p2p/discovery_test.go
🔇 Additional comments (4)
p2p/discovery.go (4)
143-204: LGTM: Robust gossip implementation
The gossipPeers implementation includes several good practices:
- Proper concurrency control with semaphores
- Bounded read limits
- Appropriate timeout handling
- Good error handling and logging
69-71:
Enhance Stop method cleanup
The Stop method should clean up resources by removing the stream handler.
func (pd *PeerDiscovery) Stop() {
+ pd.host.RemoveStreamHandler(DiscoveryProtocol)
close(pd.closeChan)
}Likely invalid or redundant comment.
60-64: 🛠️ Refactor suggestion
Make warmup configuration customizable
The hardcoded warmup logic might not be suitable for all environments.
Consider making it configurable:
type PeerDiscovery struct {
// ... existing fields ...
+ warmupRounds int
}
- //before periodic gossip, start two rounds of warmup
- pd.gossipPeers(ctx)
- time.Sleep(1 * time.Second)
- pd.gossipPeers(ctx)
+ // Perform warmup rounds if configured
+ for i := 0; i < pd.warmupRounds; i++ {
+ pd.gossipPeers(ctx)
+ if i < pd.warmupRounds-1 {
+ time.Sleep(1 * time.Second)
+ }
+ }Likely invalid or redundant comment.
42-44:
Add handler registration check
Multiple instances of PeerDiscovery with the same host could override existing handlers.
-// Set up discovery protocol handler
-h.SetStreamHandler(DiscoveryProtocol, pd.handleDiscovery)
+// Register handler only if not already set
+if h.Mux().FindHandlerByProtocol(DiscoveryProtocol) == nil {
+ h.SetStreamHandler(DiscoveryProtocol, pd.handleDiscovery)
+} else {
+ pd.logger.Warn().Msg("Discovery protocol handler already registered")
+}Likely invalid or redundant comment.
|
@gartnera unit test 1.20 go version does not have slices package and it's blocking 1.22 unit test as well |
Yeah I think it's ok to remove. Maybe do it in a separate PR in case this one drags on for awhile though. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- p2p/discovery.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: test go 1.20
p2p/discovery.go
[failure] 7-7:
package slices is not in GOROOT (/opt/hostedtoolcache/go/1.20.14/x64/src/slices)
🔇 Additional comments (6)
p2p/discovery.go (6)
216-216:
Avoid logging raw peer data
Logging raw peer data may expose sensitive information in logs.
-pd.logger.Debug().Msgf("Received peer data: %s", string(buf))
+pd.logger.Debug().Msgf("Received peer data from peer %s", p.ID)Likely invalid or redundant comment.
23-23:
Convert GossipInterval to a constant
To prevent potential race conditions and maintain immutability, declare GossipInterval as a constant.
-var GossipInterval = 30 * time.Second
+const GossipInterval = 30 * time.SecondLikely invalid or redundant comment.
73-75:
Add cleanup in Stop method
The Stop method should clean up resources by removing the stream handler.
func (pd *PeerDiscovery) Stop() {
+ pd.host.RemoveStreamHandler(DiscoveryProtocol)
close(pd.closeChan)
}Likely invalid or redundant comment.
43-45:
Check for existing handler before registration
Multiple instances of PeerDiscovery with the same host could override existing handlers.
-// Set up discovery protocol handler
-h.SetStreamHandler(DiscoveryProtocol, pd.handleDiscovery)
+// Register handler only if not already set
+if h.Mux().FindHandlerByProtocol(DiscoveryProtocol) == nil {
+ h.SetStreamHandler(DiscoveryProtocol, pd.handleDiscovery)
+} else {
+ pd.logger.Warn().Msg("Discovery protocol handler already registered")
+}Likely invalid or redundant comment.
64-68: 🛠️ Refactor suggestion
Make warmup configuration customizable
The hardcoded warmup logic might not be suitable for all environments.
type PeerDiscovery struct {
host host.Host
knownPeers map[peer.ID]peer.AddrInfo
bootstrapPeers []peer.AddrInfo
mu sync.RWMutex
logger zerolog.Logger
closeChan chan struct{}
+ warmupRounds int
}
- //before periodic gossip, start two rounds of warmup
- pd.gossipPeers(ctx)
- time.Sleep(1 * time.Second)
- pd.gossipPeers(ctx)
+ // Perform warmup rounds if configured
+ for i := 0; i < pd.warmupRounds; i++ {
+ pd.gossipPeers(ctx)
+ if i < pd.warmupRounds-1 {
+ time.Sleep(1 * time.Second)
+ }
+ }Likely invalid or redundant comment.
21-21: 🛠️ Refactor suggestion
Consider making MaxGossipConcurrency configurable
The hardcoded concurrency limit might not be optimal for all deployments.
-const MaxGossipConcurrency = 50
+var DefaultMaxGossipConcurrency = 50
type PeerDiscovery struct {
host host.Host
knownPeers map[peer.ID]peer.AddrInfo
bootstrapPeers []peer.AddrInfo
mu sync.RWMutex
logger zerolog.Logger
closeChan chan struct{}
+ maxConcurrency int
}
func NewPeerDiscovery(h host.Host, bootstrapPeers []peer.AddrInfo, opts ...Option) *PeerDiscovery {
pd := &PeerDiscovery{
host: h,
knownPeers: make(map[peer.ID]peer.AddrInfo),
bootstrapPeers: bootstrapPeers,
logger: log.With().Str("module", "peer-discovery").Logger(),
closeChan: make(chan struct{}),
+ maxConcurrency: DefaultMaxGossipConcurrency,
}
for _, opt := range opts {
opt(pd)
}
return pd
}
+type Option func(*PeerDiscovery)
+
+func WithMaxConcurrency(n int) Option {
+ return func(pd *PeerDiscovery) {
+ if n > 0 {
+ pd.maxConcurrency = n
+ }
+ }
+}Likely invalid or redundant comment.
but it's blocking checks in this PR |
Yeah we do it quickly in a separate PR, then rebase this one |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
p2p/communication.go (1)
62-62: Add documentation for the discovery field.The new
discoveryfield lacks documentation explaining its purpose and lifecycle management.+ // discovery manages the private peer discovery mechanism discovery *PeerDiscovery
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
p2p/communication.go(6 hunks)p2p/communication_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- p2p/communication_test.go
🔇 Additional comments (3)
p2p/communication.go (3)
Line range hint 251-309: LGTM: Host configuration with proper resource management.
The host configuration is well-implemented with:
- Appropriate resource limits
- Connection manager
- Whitelist gating
- Relay disabled for security
425-427: LGTM: Proper cleanup in Stop method.
The Stop method correctly cleans up the discovery resources before closing the host.
341-356:
Improve peer discovery initialization and error handling.
The current implementation has several issues:
- No validation for empty bootstrap peers list
- Using background context without cancellation
- No error handling for discovery start
Apply this diff to improve the implementation:
c.logger.Info().Msg("Start peer discovery/gossip...")
bootstrapPeerAddrInfos := make([]peer.AddrInfo, 0, len(c.bootstrapPeers))
for _, addr := range c.bootstrapPeers {
peerInfo, err := peer.AddrInfoFromP2pAddr(addr)
if err != nil {
c.logger.Error().Err(err).Msgf("fail to convert multiaddr to peer info: %s", addr)
continue
}
bootstrapPeerAddrInfos = append(bootstrapPeerAddrInfos, *peerInfo)
}
+if len(bootstrapPeerAddrInfos) == 0 {
+ return fmt.Errorf("no valid bootstrap peers available for discovery")
+}
-discovery := NewPeerDiscovery(c.host, bootstrapPeerAddrInfos)
-c.discovery = discovery
-discovery.Start(context.Background())
+c.discovery = NewPeerDiscovery(c.host, bootstrapPeerAddrInfos)
+ctx, cancel := context.WithCancel(context.Background())
+c.discoveryCancel = cancel
+if err := c.discovery.Start(ctx); err != nil {
+ cancel()
+ return fmt.Errorf("failed to start peer discovery: %w", err)
+}Likely invalid or redundant comment.
should be used/merged with #31.
The public DHT can bring in unwanted connections that can overwhelm resources.
This PR replaces public libp2p K/DHT with custom private peer discovery via gossip among
the authenticated peers (see #31).
This addresses the issue that removing DHT can break the performance e2e test (since >2
zetaclients would not be able to discover other zetaclients via DHT peer discovery, thus
Keygen/Keysign will fail).
This discovery mechanism is adaptive and does not require any configuration except a
common bootstrap node upon startup. When it fails, zetaclients can fall back to hardcode the
correct peer information in the address_book.
Reviewer notes: because DHT rendezvous is removed, the interface of Communicator has
the field
rendezvousremoved. This affects most of the changes in the calling site and test cases.Summary by CodeRabbit
New Features
PeerDiscoverystruct for improved peer management in the P2P network.Bug Fixes
Refactor
NewCommunicationandNewTssfunction signatures by removing unnecessary parameters.Tests
Chores